feat: Add Global Navbar Search#938
feat: Add Global Navbar Search#938ChiragTrivedi06 wants to merge 4 commits intoalphaonelabs:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a global search API endpoint and client-side navbar search UI; also sets default ordering for Goods and Order models to descending by creation time via model Meta and a Django migration. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant JS as Navbar JS
participant Server as Django (/api/search/)
participant DB as Database (Subjects/Courses)
Browser->>JS: User types query
JS->>JS: Debounce input (300ms)
JS->>Server: GET /api/search/?q=QUERY
Server->>DB: Query Subjects (name) & Courses (title/tags), filter published
DB-->>Server: Result sets (subjects, courses)
Server-->>JS: JSON { results: [...] }
JS-->>Browser: Render navbar-search-results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/models.py (2)
1631-1651: 🛠️ Refactor suggestion | 🟠 MajorSame DJ012 + RUF012 violations in
Order.MetaThe
Metaclass (lines 1642–1643) is added after bothsave()(line 1631) andgenerate_tracking_number()(line 1639), again violating the Django Style Guide ordering rule. Apply the same fix: moveMetato immediately after the field declarations and use a tuple.♻️ Proposed fix
updated_at = models.DateTimeField(auto_now=True) + class Meta: + ordering = ("-created_at",) + def save(self, *args, **kwargs): ... def generate_tracking_number(self): return f"TRACK-{self.pk}-{int(time.time())}-{uuid.uuid4().hex[:6].upper()}" - class Meta: - ordering = ["-created_at"] - def __str__(self): return f"Order #{self.id} ({self.user.email})"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/models.py` around lines 1631 - 1651, The Meta class for the Order model is misplaced after method definitions and uses a list for ordering; move the class Meta so it sits immediately after the model field declarations (i.e., before methods like save and generate_tracking_number) and change ordering = ["-created_at"] to a tuple ordering = ("-created_at",) to conform to the Django style guide and avoid DJ012/RUF012 violations; update references to Order.Meta location only—no logic changes to save(), generate_tracking_number(), or other methods.
1308-1325: 🛠️ Refactor suggestion | 🟠 Major
Metaclass must be repositioned beforesave()— DJ012 + RUF012Two Ruff violations are flagged here:
- DJ012: Per the Django Style Guide,
Metamust appear after field declarations and before any method definitions (clean,save,__str__, properties). The current placement (aftersave()) inverts the required order.- RUF012:
orderingis a mutable list class attribute; use a tuple to satisfy the linter.As per coding guidelines: fix linting errors in code.
♻️ Proposed fix — reposition
Metaand use a tuplesku = models.SlugField(unique=True, blank=True) created_at = models.DateTimeField(auto_now_add=True) updated_at = models.DateTimeField(auto_now=True) + class Meta: + ordering = ("-created_at",) + `@property` def image_url(self): ... def save(self, *args, **kwargs): ... super().save(*args, **kwargs) - class Meta: - ordering = ["-created_at"] - def __str__(self): return f"{self.name} (${self.price})"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/models.py` around lines 1308 - 1325, Move the Meta class so it appears before any method definitions (i.e., place the Meta block above the save() and __str__ methods) to satisfy Django ordering (DJ012), and change the ordering attribute from a mutable list to an immutable tuple (ordering = ("-created_at",)) to satisfy RUF012; ensure this Meta repositioning is applied within the same model class that defines save(), __str__, and the sku/slug logic so references like save() and Goods.objects.filter(slug=...) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/migrations/0064_alter_goods_options_alter_order_options.py`:
- Around line 8-21: Do not modify the autogenerated migration's dependencies or
operations lists; instead suppress RUF012 for migration files via the Ruff
config by adding a per-file-ignores entry that targets your migrations (e.g.,
"web/migrations/*.py" or "*/migrations/*.py") and ignores RUF012; update
pyproject.toml or ruff.toml accordingly so the linter skips the mutable class
attribute warning for the generated symbols dependencies and operations in
migration files.
In `@web/urls.py`:
- Line 105: The fetch to the search endpoint is hardcoded and will 404 under
i18n prefixes; update the template (web/templates/base.html) to resolve the URL
with Django's url tag and expose it to JS (e.g. add a data-search-url attribute
on the search input using {% url 'global_search_api' %}), then change the JS
that currently calls fetch('/api/search/?q=...') to read that attribute (e.g.
input.getAttribute('data-search-url')) and call
fetch(`${searchUrl}?q=${encodeURIComponent(query)}`) so global_search_api is
correctly resolved for all language prefixes.
In `@web/views.py`:
- Around line 8869-8871: The Course queryset line exceeds the 120-char limit;
refactor the call to Course.objects.filter(...) by breaking the arguments across
multiple lines using parentheses so each line is under 120 chars — e.g., place
the Q(title__icontains=query) | Q(tags__icontains=query) expression and the
status="published" kwarg on separate indented lines inside the filter invocation
(referencing Course.objects.filter and Q) and keep the slice [:5] on its own
line or together with the closing parenthesis to maintain readability.
- Around line 8849-8854: Add the `@require_GET` decorator to the global_search_api
view and add type hints: annotate the parameter as request: HttpRequest and the
return as -> JsonResponse in the global_search_api function signature; also
ensure require_GET is imported from django.views.decorators.http and
HttpRequest/JsonResponse are imported from django.http if not already present so
the endpoint is explicitly GET-only and satisfies typing requirements.
---
Outside diff comments:
In `@web/models.py`:
- Around line 1631-1651: The Meta class for the Order model is misplaced after
method definitions and uses a list for ordering; move the class Meta so it sits
immediately after the model field declarations (i.e., before methods like save
and generate_tracking_number) and change ordering = ["-created_at"] to a tuple
ordering = ("-created_at",) to conform to the Django style guide and avoid
DJ012/RUF012 violations; update references to Order.Meta location only—no logic
changes to save(), generate_tracking_number(), or other methods.
- Around line 1308-1325: Move the Meta class so it appears before any method
definitions (i.e., place the Meta block above the save() and __str__ methods) to
satisfy Django ordering (DJ012), and change the ordering attribute from a
mutable list to an immutable tuple (ordering = ("-created_at",)) to satisfy
RUF012; ensure this Meta repositioning is applied within the same model class
that defines save(), __str__, and the sku/slug logic so references like save()
and Goods.objects.filter(slug=...) remain unchanged.
| dependencies = [ | ||
| ('web', '0063_virtualclassroom_virtualclassroomcustomization_and_more'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AlterModelOptions( | ||
| name='goods', | ||
| options={'ordering': ['-created_at']}, | ||
| ), | ||
| migrations.AlterModelOptions( | ||
| name='order', | ||
| options={'ordering': ['-created_at']}, | ||
| ), | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Suppress RUF012 for migration files via Ruff config rather than editing auto-generated code.
Both dependencies (lines 8–10) and operations (lines 12–21) are flagged by Ruff's RUF012 rule ("Mutable class attribute should be annotated with typing.ClassVar"). This is a well-known false positive for Django migration files — Django's framework intentionally reads these as mutable class-level lists, and annotating them with ClassVar in every generated file is impractical.
The idiomatic fix is a one-time Ruff config change:
⚙️ Suppress RUF012 for all migration files via per-file-ignores
In pyproject.toml (or ruff.toml):
[tool.ruff.lint.per-file-ignores]
+"*/migrations/*.py" = ["RUF012"]🧰 Tools
🪛 Ruff (0.15.1)
[warning] 8-10: Mutable default value for class attribute
(RUF012)
[warning] 12-21: Mutable default value for class attribute
(RUF012)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/migrations/0064_alter_goods_options_alter_order_options.py` around lines
8 - 21, Do not modify the autogenerated migration's dependencies or operations
lists; instead suppress RUF012 for migration files via the Ruff config by adding
a per-file-ignores entry that targets your migrations (e.g.,
"web/migrations/*.py" or "*/migrations/*.py") and ignores RUF012; update
pyproject.toml or ruff.toml accordingly so the linter skips the mutable class
attribute warning for the generated symbols dependencies and operations in
migration files.
| # Course Management | ||
| path("courses/create/", views.create_course, name="create_course"), | ||
| path("courses/search/", views.course_search, name="course_search"), | ||
| path("api/search/", views.global_search_api, name="global_search_api"), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the urls.py structure and confirm the i18n_patterns context
cat -n web/urls.py | head -120 | tail -40Repository: alphaonelabs/alphaonelabs-education-website
Length of output: 3229
🏁 Script executed:
# Search for how the search API is being called in the frontend
rg -n "api/search" --type html --type js -C 3Repository: alphaonelabs/alphaonelabs-education-website
Length of output: 538
🏁 Script executed:
# Check for Django URL template tag usage in base.html
rg -n "url.*global_search" --type htmlRepository: alphaonelabs/alphaonelabs-education-website
Length of output: 69
🏁 Script executed:
# Examine the i18n_patterns configuration mentioned in the review
rg -n "prefix_default_language|i18n_patterns" web/urls.py -C 2Repository: alphaonelabs/alphaonelabs-education-website
Length of output: 576
🏁 Script executed:
# Check if there are other API endpoints to see how they handle i18n URLs
rg -n "path.*api/" web/urls.py -A 1Repository: alphaonelabs/alphaonelabs-education-website
Length of output: 346
🏁 Script executed:
# Look at the broader context of how JavaScript calls other Django URLs in the templates
rg -n "fetch.*url|ajax.*url|\{% url" web/templates/base.html -C 2 | head -50Repository: alphaonelabs/alphaonelabs-education-website
Length of output: 4492
🏁 Script executed:
# Check if map-data API is also hardcoded or properly handled with template tags
rg -n "api/map-data|map_data" web/templates/base.html -C 2Repository: alphaonelabs/alphaonelabs-education-website
Length of output: 69
🏁 Script executed:
# Verify the full context of the search implementation to understand severity
cat -n web/templates/base.html | sed -n '1110,1135p'Repository: alphaonelabs/alphaonelabs-education-website
Length of output: 1895
Use Django template tag to resolve the search API URL in JavaScript to support i18n URL prefixes.
The endpoint is registered inside i18n_patterns with prefix_default_language=True (line 629), meaning its effective URL is /en/api/search/, /fr/api/search/, etc., not /api/search/. The current hardcoded fetch call on line 1121 of web/templates/base.html (fetch('/api/search/?q=...')) will silently fail with a 404 in any non-default language context.
Resolve the URL dynamically using Django's template tag. For example, wrap the search input in a data attribute:
Suggested approach
<input type="text" data-search-url="{% url 'global_search_api' %}" ...>Then reference it in JavaScript:
const searchUrl = input.getAttribute('data-search-url');
fetch(`${searchUrl}?q=${encodeURIComponent(query)}`)This matches the existing pattern used throughout the template for all other route references (e.g., {% url 'index' %}, {% url 'course_search' %}).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/urls.py` at line 105, The fetch to the search endpoint is hardcoded and
will 404 under i18n prefixes; update the template (web/templates/base.html) to
resolve the URL with Django's url tag and expose it to JS (e.g. add a
data-search-url attribute on the search input using {% url 'global_search_api'
%}), then change the JS that currently calls fetch('/api/search/?q=...') to read
that attribute (e.g. input.getAttribute('data-search-url')) and call
fetch(`${searchUrl}?q=${encodeURIComponent(query)}`) so global_search_api is
correctly resolved for all language prefixes.
| def global_search_api(request): | ||
| """API endpoint for global search of subjects and courses.""" | ||
| query = request.GET.get("q", "").strip() | ||
| if not query or len(query) < 2: | ||
| return JsonResponse({"results": []}) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine web/views.py
fd "views.py" --path "*/web/*"Repository: alphaonelabs/alphaonelabs-education-website
Length of output: 302
🏁 Script executed:
# Get the file size to understand how to read it efficiently
wc -l web/views.pyRepository: alphaonelabs/alphaonelabs-education-website
Length of output: 104
🏁 Script executed:
# Read the specific section mentioned in the review (lines 8849-8854 and surrounding context)
# Let's read a broader range to see the full function implementation
sed -n '8845,8885p' web/views.py | cat -nRepository: alphaonelabs/alphaonelabs-education-website
Length of output: 1477
🏁 Script executed:
# Check if there are any lines exceeding 120 characters in the function
sed -n '8849,8881p' web/views.py | awk 'length > 120 {print NR+8848 ": " length " chars: " $0}'Repository: alphaonelabs/alphaonelabs-education-website
Length of output: 69
🏁 Script executed:
# Check imports at the top of web/views.py to see what's available
head -50 web/views.py | cat -nRepository: alphaonelabs/alphaonelabs-education-website
Length of output: 2025
🏁 Script executed:
# Search for `@require_GET` usage in the file to see the pattern
rg "@require_GET" web/views.py -B 2 -A 2Repository: alphaonelabs/alphaonelabs-education-website
Length of output: 380
Add a GET-only decorator and type hints for the new API.
Line 8849 should include type hints and the @require_GET decorator to satisfy the repository's typing requirements and establish the GET-only contract for this API endpoint.
Suggested change
+@require_GET
-def global_search_api(request):
+def global_search_api(request: HttpRequest) -> JsonResponse:
"""API endpoint for global search of subjects and courses."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/views.py` around lines 8849 - 8854, Add the `@require_GET` decorator to the
global_search_api view and add type hints: annotate the parameter as request:
HttpRequest and the return as -> JsonResponse in the global_search_api function
signature; also ensure require_GET is imported from django.views.decorators.http
and HttpRequest/JsonResponse are imported from django.http if not already
present so the endpoint is explicitly GET-only and satisfies typing
requirements.
| # Search Courses | ||
| courses = Course.objects.filter(Q(title__icontains=query) | Q(tags__icontains=query), status="published")[:5] | ||
| for course in courses: |
There was a problem hiding this comment.
Wrap the course filter to stay within the 120‑char limit.
Line 8870 exceeds the configured maximum line length.
✅ Suggested change
- courses = Course.objects.filter(Q(title__icontains=query) | Q(tags__icontains=query), status="published")[:5]
+ courses = Course.objects.filter(
+ Q(title__icontains=query) | Q(tags__icontains=query),
+ status="published",
+ )[:5]As per coding guidelines: Maximum Python line length is 120 characters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/views.py` around lines 8869 - 8871, The Course queryset line exceeds the
120-char limit; refactor the call to Course.objects.filter(...) by breaking the
arguments across multiple lines using parentheses so each line is under 120
chars — e.g., place the Q(title__icontains=query) | Q(tags__icontains=query)
expression and the status="published" kwarg on separate indented lines inside
the filter invocation (referencing Course.objects.filter and Q) and keep the
slice [:5] on its own line or together with the closing parenthesis to maintain
readability.
|
@A1L13N Could you please review this Pr and merge it |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/templates/base.html`:
- Line 354: Replace the JS-only CSS selectors by adding data attributes to the
desktop and mobile search input and results elements (e.g. change
class="navbar-search-input" to data-navbar-search-input and
class="navbar-search-results" to data-navbar-search-results) and then update the
JS that currently queries ".navbar-search-input" and ".navbar-search-results" to
use document.querySelector('[data-navbar-search-input]') /
'[data-navbar-search-results]' (and the same for querySelectorAll or closest
uses) so the styling classes are not used as JS hooks; apply the change
consistently for both desktop and mobile inputs/results and update all JS
references that select these elements.
- Around line 1155-1158: The shared debounceTimer declared outside the loop
causes cross-input cancellation; move the timer into the per-input closure so
each `.navbar-search-input` gets its own timer. Specifically, inside the
searchInputs.forEach(input => { ... }) block, declare a local let debounceTimer
(or const createDebounce) and use that timer in the input event listener instead
of the outer debounceTimer so clearTimeout/setTimeout only affect the current
input's pending request.
- Around line 1209-1215: The click-outside guard in the document click handler
is too broad because it checks event.target.closest('.relative'), which matches
many unrelated navbar elements; update the guard inside the
document.addEventListener('click', ...) handler to only treat clicks inside the
actual search component(s) as in-scope (e.g., use a specific selector like
closest('.navbar-search') or a dedicated data attribute such as
closest('[data-navbar-search]') or closest('.navbar-search-results')), and leave
the existing logic that hides elements matching '.navbar-search-results'
unchanged so only clicks outside the real search container hide the results.
- Around line 1158-1206: Add the required ARIA attributes to the search input
and results container to implement the live-results combobox pattern: ensure the
input elements (the ones iterated by searchInputs in the searchInputs.forEach
block) include role="combobox", aria-autocomplete="list", aria-expanded="false"
and aria-controls pointing to a unique id for their corresponding
resultsContainer, and give the resultsContainer element role="listbox" and that
same id; in the JS (inside the input 'input' handler and the Escape key handler)
toggle the input's aria-expanded between "true"/"false" when you show/hide
resultsContainer and update resultsContainer.innerHTML as before so screen
readers can navigate the dynamic list.
- Around line 1172-1196: The fetch block that calls
fetch(`/api/search/?q=${encodeURIComponent(query)}`) lacks error handling and a
response.ok check, so add a guard after the fetch to verify response.ok before
calling response.json() and handle non-OK responses (e.g., show a friendly "No
results" or error message in resultsContainer); also chain a .catch(...) to the
promise to handle network/parse errors and ensure resultsContainer is hidden or
displays an error instead of leaving the UI broken, and when using the parsed
data check that data && Array.isArray(data.results) before accessing
data.results.length (update the anonymous then callbacks around response.json()
and the subsequent data handler accordingly).
- Around line 374-386: The badge currently chooses {{ item_count }} whenever
request.user.is_authenticated even if item_count == 0 while s_count > 0; update
the span rendering logic so that when request.user.is_authenticated you display
s_count if item_count is zero (or prefer s_count when >0), otherwise display
item_count — i.e. make the inner branch consider both item_count and s_count
(use a nested conditional that shows s_count when item_count == 0 and s_count >
0); apply the same change to the mobile block that uses the same variables at
the other location.
- Around line 1176-1193: The code injects unsanitized values into innerHTML
(resultsContainer.innerHTML) using query, result.title, result.type, result.url,
and result.icon which enables XSS; fix by replacing the template-literal
innerHTML construction with explicit DOM creation: for each item in
data.results, create elements via document.createElement, set text nodes via
textContent for title/type and use element.setAttribute('href', validatedUrl)
after validating result.url only allows http(s) or safe relative paths, and only
apply result.icon to className after validating/whitelisting allowed icon class
tokens (or escape/omit unsafe characters); for the "no results" branch create a
div and set its textContent to include the query safely rather than
interpolating into innerHTML, then append the constructed nodes to
resultsContainer and remove any use of innerHTML to prevent attribute/event
injection.
| autocomplete="off" | ||
| placeholder="What do you want to learn?" | ||
| class="rounded-full w-[250px] bg-white dark:bg-gray-700 text-gray-900 dark:text-gray-100 px-3 py-1.5 focus:outline-none focus:ring-2 focus:ring-teal-300 dark:focus:ring-teal-700" /> | ||
| class="navbar-search-input rounded-full w-[250px] bg-white dark:bg-gray-700 text-gray-900 dark:text-gray-100 px-3 py-1.5 focus:outline-none focus:ring-2 focus:ring-teal-300 dark:focus:ring-teal-700" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace custom CSS classes with data-* attributes for JavaScript hooks.
navbar-search-input (lines 354, 563) and navbar-search-results (lines 360, 569) are custom CSS class names added solely as JS selectors, violating the project guideline. Use data-* attributes instead and update the JS selectors accordingly.
♻️ Proposed fix
In the desktop search input (line 354):
-class="navbar-search-input rounded-full w-[250px] ..."
+data-search-input class="rounded-full w-[250px] ..."In the desktop results container (line 360):
-<div class="navbar-search-results hidden absolute top-full ...">
+<div data-search-results class="hidden absolute top-full ...">Apply the same changes at lines 563 and 569 for the mobile equivalents.
In the JavaScript (lines 1155, 1159, 1211):
-const searchInputs = document.querySelectorAll('.navbar-search-input');
+const searchInputs = document.querySelectorAll('[data-search-input]');
...
-const resultsContainer = input.closest('.relative').querySelector('.navbar-search-results');
+const resultsContainer = input.closest('.relative').querySelector('[data-search-results]');
...
-document.querySelectorAll('.navbar-search-results').forEach(...)
+document.querySelectorAll('[data-search-results]').forEach(...)As per coding guidelines: "Never use custom CSS classes" (**/*.{html,htm,css}).
Also applies to: 360-360, 563-563, 569-569
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/templates/base.html` at line 354, Replace the JS-only CSS selectors by
adding data attributes to the desktop and mobile search input and results
elements (e.g. change class="navbar-search-input" to data-navbar-search-input
and class="navbar-search-results" to data-navbar-search-results) and then update
the JS that currently queries ".navbar-search-input" and
".navbar-search-results" to use
document.querySelector('[data-navbar-search-input]') /
'[data-navbar-search-results]' (and the same for querySelectorAll or closest
uses) so the styling classes are not used as JS hooks; apply the change
consistently for both desktop and mobile inputs/results and update all JS
references that select these elements.
| {% with item_count=request.user.cart.item_count|default:0 %} | ||
| {% with s_count=request.session.session_key|get_cart_item_count|default:0 %} | ||
| {% if item_count > 0 or s_count > 0 %} | ||
| <span class="absolute -top-1 -right-1 bg-orange-500 text-white text-xs rounded-full h-4 w-4 flex items-center justify-center"> | ||
| {% if request.user.is_authenticated %} | ||
| {{ item_count }} | ||
| {% else %} | ||
| {{ s_count }} | ||
| {% endif %} | ||
| </span> | ||
| {% endif %} | ||
| </span> | ||
| {% endif %} | ||
| {% endwith %} | ||
| {% endwith %} |
There was a problem hiding this comment.
Cart badge can render 0 for an authenticated user when only session items exist.
If item_count = 0 and s_count > 0 (e.g., items were added before login), the outer {% if %} fires but the inner branch displays item_count (0). The same issue exists in the mobile equivalent at lines 777–789.
🐛 Proposed fix (desktop; apply the same to the mobile block at lines 777–789)
-{% with item_count=request.user.cart.item_count|default:0 %}
- {% with s_count=request.session.session_key|get_cart_item_count|default:0 %}
- {% if item_count > 0 or s_count > 0 %}
- <span class="absolute -top-1 -right-1 bg-orange-500 text-white text-xs rounded-full h-4 w-4 flex items-center justify-center">
- {% if request.user.is_authenticated %}
- {{ item_count }}
- {% else %}
- {{ s_count }}
- {% endif %}
- </span>
- {% endif %}
- {% endwith %}
-{% endwith %}
+{% if request.user.is_authenticated %}
+ {% with item_count=request.user.cart.item_count|default:0 %}
+ {% if item_count > 0 %}
+ <span class="absolute -top-1 -right-1 bg-orange-500 text-white text-xs rounded-full h-4 w-4 flex items-center justify-center">
+ {{ item_count }}
+ </span>
+ {% endif %}
+ {% endwith %}
+{% else %}
+ {% with s_count=request.session.session_key|get_cart_item_count|default:0 %}
+ {% if s_count > 0 %}
+ <span class="absolute -top-1 -right-1 bg-orange-500 text-white text-xs rounded-full h-4 w-4 flex items-center justify-center">
+ {{ s_count }}
+ </span>
+ {% endif %}
+ {% endwith %}
+{% endif %}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {% with item_count=request.user.cart.item_count|default:0 %} | |
| {% with s_count=request.session.session_key|get_cart_item_count|default:0 %} | |
| {% if item_count > 0 or s_count > 0 %} | |
| <span class="absolute -top-1 -right-1 bg-orange-500 text-white text-xs rounded-full h-4 w-4 flex items-center justify-center"> | |
| {% if request.user.is_authenticated %} | |
| {{ item_count }} | |
| {% else %} | |
| {{ s_count }} | |
| {% endif %} | |
| </span> | |
| {% endif %} | |
| </span> | |
| {% endif %} | |
| {% endwith %} | |
| {% endwith %} | |
| {% if request.user.is_authenticated %} | |
| {% with item_count=request.user.cart.item_count|default:0 %} | |
| {% if item_count > 0 %} | |
| <span class="absolute -top-1 -right-1 bg-orange-500 text-white text-xs rounded-full h-4 w-4 flex items-center justify-center"> | |
| {{ item_count }} | |
| </span> | |
| {% endif %} | |
| {% endwith %} | |
| {% else %} | |
| {% with s_count=request.session.session_key|get_cart_item_count|default:0 %} | |
| {% if s_count > 0 %} | |
| <span class="absolute -top-1 -right-1 bg-orange-500 text-white text-xs rounded-full h-4 w-4 flex items-center justify-center"> | |
| {{ s_count }} | |
| </span> | |
| {% endif %} | |
| {% endwith %} | |
| {% endif %} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/templates/base.html` around lines 374 - 386, The badge currently chooses
{{ item_count }} whenever request.user.is_authenticated even if item_count == 0
while s_count > 0; update the span rendering logic so that when
request.user.is_authenticated you display s_count if item_count is zero (or
prefer s_count when >0), otherwise display item_count — i.e. make the inner
branch consider both item_count and s_count (use a nested conditional that shows
s_count when item_count == 0 and s_count > 0); apply the same change to the
mobile block that uses the same variables at the other location.
| const searchInputs = document.querySelectorAll('.navbar-search-input'); | ||
| let debounceTimer; | ||
|
|
||
| searchInputs.forEach(input => { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Shared debounceTimer across multiple inputs can cause cross-input cancellation.
The single debounceTimer variable is closed over by every input's input event listener. A keystroke in one input will clearTimeout the pending debounce started by the other, preventing the first request from firing. Move the timer inside the forEach closure so each input gets its own:
♻️ Proposed fix
- const searchInputs = document.querySelectorAll('.navbar-search-input');
- let debounceTimer;
-
- searchInputs.forEach(input => {
- const resultsContainer = input.closest('.relative').querySelector('.navbar-search-results');
-
- input.addEventListener('input', function() {
- clearTimeout(debounceTimer);
+ document.querySelectorAll('[data-search-input]').forEach(input => {
+ let debounceTimer;
+ const resultsContainer = input.closest('.relative').querySelector('[data-search-results]');
+
+ input.addEventListener('input', function() {
+ clearTimeout(debounceTimer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/templates/base.html` around lines 1155 - 1158, The shared debounceTimer
declared outside the loop causes cross-input cancellation; move the timer into
the per-input closure so each `.navbar-search-input` gets its own timer.
Specifically, inside the searchInputs.forEach(input => { ... }) block, declare a
local let debounceTimer (or const createDebounce) and use that timer in the
input event listener instead of the outer debounceTimer so
clearTimeout/setTimeout only affect the current input's pending request.
| searchInputs.forEach(input => { | ||
| const resultsContainer = input.closest('.relative').querySelector('.navbar-search-results'); | ||
|
|
||
| input.addEventListener('input', function() { | ||
| clearTimeout(debounceTimer); | ||
| const query = this.value.trim(); | ||
|
|
||
| if (query.length < 2) { | ||
| resultsContainer.innerHTML = ''; | ||
| resultsContainer.classList.add('hidden'); | ||
| return; | ||
| } | ||
|
|
||
| debounceTimer = setTimeout(() => { | ||
| fetch(`/api/search/?q=${encodeURIComponent(query)}`) | ||
| .then(response => response.json()) | ||
| .then(data => { | ||
| if (data.results.length > 0) { | ||
| resultsContainer.innerHTML = data.results.map(result => ` | ||
| <a href="${result.url}" class="flex items-center px-4 py-3 hover:bg-teal-50 dark:hover:bg-teal-900/30 border-b border-gray-100 dark:border-gray-700 last:border-0 transition-colors"> | ||
| <div class="flex-shrink-0 w-8 h-8 flex items-center justify-center rounded-full bg-teal-100 dark:bg-teal-900 text-teal-600 dark:text-teal-400 mr-3"> | ||
| <i class="${result.icon}"></i> | ||
| </div> | ||
| <div> | ||
| <div class="text-sm font-semibold text-gray-900 dark:text-gray-100">${result.title}</div> | ||
| <div class="text-xs text-gray-500 dark:text-gray-400 capitalize">${result.type}</div> | ||
| </div> | ||
| </a> | ||
| `).join(''); | ||
| resultsContainer.classList.remove('hidden'); | ||
| } else { | ||
| resultsContainer.innerHTML = ` | ||
| <div class="px-4 py-3 text-sm text-gray-500 dark:text-gray-400 text-center"> | ||
| No results found for "${query}" | ||
| </div> | ||
| `; | ||
| resultsContainer.classList.remove('hidden'); | ||
| } | ||
| }); | ||
| }, 300); | ||
| }); | ||
|
|
||
| // Close results on escape | ||
| input.addEventListener('keydown', function(e) { | ||
| if (e.key === 'Escape') { | ||
| resultsContainer.classList.add('hidden'); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Search combobox is missing required ARIA attributes for screen readers.
The live-results pattern requires role="combobox", aria-expanded, aria-autocomplete, and aria-controls on the input, plus role="listbox" on the results container. Without these, assistive technologies cannot announce or navigate the dynamic results.
Minimum additions to the input elements (lines 350–354 and 559–563):
<input type="text"
name="q"
autocomplete="off"
placeholder="What do you want to learn?"
+ role="combobox"
+ aria-autocomplete="list"
+ aria-expanded="false"
+ aria-controls="desktop-search-results"
class="..." />Results container (line 360):
-<div class="navbar-search-results hidden absolute ...">
+<div id="desktop-search-results" role="listbox" class="hidden absolute ...">Also toggle aria-expanded in the JS when results appear/disappear:
+input.setAttribute('aria-expanded', 'true');
resultsContainer.classList.remove('hidden');+input.setAttribute('aria-expanded', 'false');
resultsContainer.classList.add('hidden');As per coding guidelines: "Include proper ARIA labels where needed for accessibility" (web/templates/**/*.html).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/templates/base.html` around lines 1158 - 1206, Add the required ARIA
attributes to the search input and results container to implement the
live-results combobox pattern: ensure the input elements (the ones iterated by
searchInputs in the searchInputs.forEach block) include role="combobox",
aria-autocomplete="list", aria-expanded="false" and aria-controls pointing to a
unique id for their corresponding resultsContainer, and give the
resultsContainer element role="listbox" and that same id; in the JS (inside the
input 'input' handler and the Escape key handler) toggle the input's
aria-expanded between "true"/"false" when you show/hide resultsContainer and
update resultsContainer.innerHTML as before so screen readers can navigate the
dynamic list.
| fetch(`/api/search/?q=${encodeURIComponent(query)}`) | ||
| .then(response => response.json()) | ||
| .then(data => { | ||
| if (data.results.length > 0) { | ||
| resultsContainer.innerHTML = data.results.map(result => ` | ||
| <a href="${result.url}" class="flex items-center px-4 py-3 hover:bg-teal-50 dark:hover:bg-teal-900/30 border-b border-gray-100 dark:border-gray-700 last:border-0 transition-colors"> | ||
| <div class="flex-shrink-0 w-8 h-8 flex items-center justify-center rounded-full bg-teal-100 dark:bg-teal-900 text-teal-600 dark:text-teal-400 mr-3"> | ||
| <i class="${result.icon}"></i> | ||
| </div> | ||
| <div> | ||
| <div class="text-sm font-semibold text-gray-900 dark:text-gray-100">${result.title}</div> | ||
| <div class="text-xs text-gray-500 dark:text-gray-400 capitalize">${result.type}</div> | ||
| </div> | ||
| </a> | ||
| `).join(''); | ||
| resultsContainer.classList.remove('hidden'); | ||
| } else { | ||
| resultsContainer.innerHTML = ` | ||
| <div class="px-4 py-3 text-sm text-gray-500 dark:text-gray-400 text-center"> | ||
| No results found for "${query}" | ||
| </div> | ||
| `; | ||
| resultsContainer.classList.remove('hidden'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Unhandled fetch failures will silently break the search UI.
There is no .catch() handler and no response.ok guard. A 4xx/5xx response triggers response.json() on an error body (often HTML), throws a SyntaxError, and the rejection propagates unhandled. Even a valid JSON error body would crash at data.results.length.
🛡️ Proposed fix
fetch(`/api/search/?q=${encodeURIComponent(query)}`)
.then(response => {
+ if (!response.ok) throw new Error(`Search request failed: ${response.status}`);
return response.json();
})
.then(data => {
- if (data.results.length > 0) {
+ const results = Array.isArray(data.results) ? data.results : [];
+ if (results.length > 0) {
// … render results …
} else {
// … no results …
}
})
+ .catch(() => {
+ resultsContainer.innerHTML = '';
+ resultsContainer.classList.add('hidden');
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/templates/base.html` around lines 1172 - 1196, The fetch block that calls
fetch(`/api/search/?q=${encodeURIComponent(query)}`) lacks error handling and a
response.ok check, so add a guard after the fetch to verify response.ok before
calling response.json() and handle non-OK responses (e.g., show a friendly "No
results" or error message in resultsContainer); also chain a .catch(...) to the
promise to handle network/parse errors and ensure resultsContainer is hidden or
displays an error instead of leaving the UI broken, and when using the parsed
data check that data && Array.isArray(data.results) before accessing
data.results.length (update the anonymous then callbacks around response.json()
and the subsequent data handler accordingly).
| resultsContainer.innerHTML = data.results.map(result => ` | ||
| <a href="${result.url}" class="flex items-center px-4 py-3 hover:bg-teal-50 dark:hover:bg-teal-900/30 border-b border-gray-100 dark:border-gray-700 last:border-0 transition-colors"> | ||
| <div class="flex-shrink-0 w-8 h-8 flex items-center justify-center rounded-full bg-teal-100 dark:bg-teal-900 text-teal-600 dark:text-teal-400 mr-3"> | ||
| <i class="${result.icon}"></i> | ||
| </div> | ||
| <div> | ||
| <div class="text-sm font-semibold text-gray-900 dark:text-gray-100">${result.title}</div> | ||
| <div class="text-xs text-gray-500 dark:text-gray-400 capitalize">${result.type}</div> | ||
| </div> | ||
| </a> | ||
| `).join(''); | ||
| resultsContainer.classList.remove('hidden'); | ||
| } else { | ||
| resultsContainer.innerHTML = ` | ||
| <div class="px-4 py-3 text-sm text-gray-500 dark:text-gray-400 text-center"> | ||
| No results found for "${query}" | ||
| </div> | ||
| `; |
There was a problem hiding this comment.
Critical XSS — never inject unsanitized data into innerHTML.
Four vectors exist in this block:
- Line 1191 — direct user input:
"${query}"is the raw search query inserted intoinnerHTML. A user can type<img src=x onerror=alert(1)>and it will execute becauseinnerHTMLprocesses event-handler injections even though<script>tags are blocked. - Lines 1182–1183 —
result.titleandresult.typefrom the API injected as raw HTML. A compromised or manipulated DB record causes stored XSS. - Line 1177 —
result.urlplaced in anhrefattribute via template literal; ajavascript:URI here executes code on click. - Line 1179 —
result.iconplaced in aclassattribute; an injected" onclick="payload escapes the attribute.
Use DOM construction with textContent / setAttribute and validate URLs instead of template-literal innerHTML:
🔒 Proposed safe DOM builder
-resultsContainer.innerHTML = data.results.map(result => `
- <a href="${result.url}" class="flex items-center px-4 py-3 ...">
- <div class="...">
- <i class="${result.icon}"></i>
- </div>
- <div>
- <div class="...">${result.title}</div>
- <div class="...">${result.type}</div>
- </div>
- </a>
-`).join('');
+resultsContainer.innerHTML = '';
+data.results.forEach(result => {
+ const safeUrl = (result.url.startsWith('/') || result.url.startsWith('http')) ? result.url : '#';
+ const a = document.createElement('a');
+ a.href = safeUrl;
+ a.className = 'flex items-center px-4 py-3 hover:bg-teal-50 dark:hover:bg-teal-900/30 border-b border-gray-100 dark:border-gray-700 last:border-0 transition-colors';
+ const iconWrapper = document.createElement('div');
+ iconWrapper.className = 'flex-shrink-0 w-8 h-8 flex items-center justify-center rounded-full bg-teal-100 dark:bg-teal-900 text-teal-600 dark:text-teal-400 mr-3';
+ const icon = document.createElement('i');
+ icon.className = String(result.icon); // class only, no HTML injection
+ iconWrapper.appendChild(icon);
+ const textWrapper = document.createElement('div');
+ const titleDiv = document.createElement('div');
+ titleDiv.className = 'text-sm font-semibold text-gray-900 dark:text-gray-100';
+ titleDiv.textContent = result.title; // safe: textContent escapes HTML
+ const typeDiv = document.createElement('div');
+ typeDiv.className = 'text-xs text-gray-500 dark:text-gray-400 capitalize';
+ typeDiv.textContent = result.type; // safe
+ textWrapper.appendChild(titleDiv);
+ textWrapper.appendChild(typeDiv);
+ a.appendChild(iconWrapper);
+ a.appendChild(textWrapper);
+ resultsContainer.appendChild(a);
+});For the "no results" message:
-resultsContainer.innerHTML = `
- <div class="...">No results found for "${query}"</div>
-`;
+const msg = document.createElement('div');
+msg.className = 'px-4 py-3 text-sm text-gray-500 dark:text-gray-400 text-center';
+msg.textContent = `No results found for "${query}"`;
+resultsContainer.innerHTML = '';
+resultsContainer.appendChild(msg);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/templates/base.html` around lines 1176 - 1193, The code injects
unsanitized values into innerHTML (resultsContainer.innerHTML) using query,
result.title, result.type, result.url, and result.icon which enables XSS; fix by
replacing the template-literal innerHTML construction with explicit DOM
creation: for each item in data.results, create elements via
document.createElement, set text nodes via textContent for title/type and use
element.setAttribute('href', validatedUrl) after validating result.url only
allows http(s) or safe relative paths, and only apply result.icon to className
after validating/whitelisting allowed icon class tokens (or escape/omit unsafe
characters); for the "no results" branch create a div and set its textContent to
include the query safely rather than interpolating into innerHTML, then append
the constructed nodes to resultsContainer and remove any use of innerHTML to
prevent attribute/event injection.
| document.addEventListener('click', function(event) { | ||
| if (!event.target.closest('.relative')) { | ||
| document.querySelectorAll('.navbar-search-results').forEach(container => { | ||
| container.classList.add('hidden'); | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Click-outside guard is too broad — closest('.relative') matches unrelated navbar elements.
.relative is used on the cart icon wrapper, user dropdown, language dropdown, and other elements throughout the page. Clicking any of them satisfies the guard, keeping the search results open indefinitely. Scope the check to the actual search containers.
🛡️ Proposed fix
-document.addEventListener('click', function(event) {
- if (!event.target.closest('.relative')) {
- document.querySelectorAll('.navbar-search-results').forEach(container => {
- container.classList.add('hidden');
- });
- }
-});
+document.addEventListener('click', function(event) {
+ // Only keep results open when the click is inside a search container
+ const isInsideSearch = event.target.closest('[data-search-input]') ||
+ event.target.closest('[data-search-results]');
+ if (!isInsideSearch) {
+ document.querySelectorAll('[data-search-results]').forEach(container => {
+ container.classList.add('hidden');
+ });
+ }
+});(This also aligns with the data-* attribute refactor suggested above.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/templates/base.html` around lines 1209 - 1215, The click-outside guard in
the document click handler is too broad because it checks
event.target.closest('.relative'), which matches many unrelated navbar elements;
update the guard inside the document.addEventListener('click', ...) handler to
only treat clicks inside the actual search component(s) as in-scope (e.g., use a
specific selector like closest('.navbar-search') or a dedicated data attribute
such as closest('[data-navbar-search]') or closest('.navbar-search-results')),
and leave the existing logic that hides elements matching
'.navbar-search-results' unchanged so only clicks outside the real search
container hide the results.
|
Hi @Shubhashish-Chakraborty, can you help me understand why this is blocking my PR? |
|
hey @ChiragTrivedi06 |
|
and then wait for the maintainers they will review and merge if everything alligns well |
💬 Unresolved Review ConversationsHi @ChiragTrivedi06! 👋 This pull request currently has 11 unresolved review conversations. Please address all review feedback and push a new commit to resolve them before this PR can be merged. Steps to resolve:
Once all conversations are resolved, this notice will be removed automatically. Thank you! 🙏 |

Related issues
Fixes #937
Overview
This PR implements a real-time global search functionality integrated directly into the platform's navigation bar.
Key Improvements
1. Global Search Infrastructure
Search API (
/api/search/)Implemented a lightweight JSON endpoint in
web/views.pythat provides optimized filtering for:Navbar UI/UX Integration
base.html2. Model Query Optimization
Resolved
UnorderedObjectListWarningduring paginationAffected models:
GoodsOrderImplementation
Added the following in
web/models.py: